[NativeAOT] Move RequiresAlign8 flag from RareFlags into ExtendedFlags #106010

filipnavara · 2024-08-06T10:06:04Z

Implementation of idea from https://github.com/dotnet/runtime/pull/105931/files#r1704319780.

The benchmark code and results are below. I tested it on Raspberry Pi. The tests were run in succession, CPU temperature was around 48 degrees, so no thermal throttling should be involved.

Predictably, the performance improves for `RequiresAlign8` cases where we don't need to decode the optional fields block. There's about 5% perf hit for non-align8 array allocations and 2% perf hit for non-align8 value type allocations. Some perf hit for arrays was expected, the other code path needs more investigation though as no hit is expected there.

Updated benchmark results here: #106010 (comment)

This would make it easier to set the RequiresAlign8 flag for truncated method tables used in GCStaticEETypeNode.

Benchmark code:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime;
using System.Runtime.CompilerServices;

public unsafe class Program
{
    [RuntimeImport("RhNewObject")]
    [MethodImpl(MethodImplOptions.InternalCall)]
    private static extern object RhNewObject(void* pMT);
    [RuntimeImport("RhNewArray")]
    [MethodImpl(MethodImplOptions.InternalCall)]
    private static extern object RhNewArray(void* pMT, int length);

    public static void Main()
    {
        var stopWatch = new System.Diagnostics.Stopwatch();

        for (int j = 0; j < 10; j++)
        {
            stopWatch.Restart();
            for (int i = 0; i < 1000000; i++)
                _ = RhNewArray((void*)typeof(int[]).TypeHandle.Value, 2);
            stopWatch.Stop();
            Console.WriteLine("NewIntArray: " + stopWatch.Elapsed);
        }

        for (int j = 0; j < 10; j++)
        {
            stopWatch.Restart();
            for (int i = 0; i < 1000000; i++)
                _ = RhNewArray((void*)typeof(double[]).TypeHandle.Value, 2);
            stopWatch.Stop();
            Console.WriteLine("NewDoubleArray: " + stopWatch.Elapsed);
        }

        for (int j = 0; j < 10; j++)
        {
            stopWatch.Restart();
            for (int i = 0; i < 1000000; i++)
                _ = RhNewObject((void*)typeof(ValueTuple<int, int>).TypeHandle.Value);
            stopWatch.Stop();
            Console.WriteLine("NewIntTuple: " + stopWatch.Elapsed);
        }

        for (int j = 0; j < 10; j++)
        {
            stopWatch.Restart();
            for (int i = 0; i < 1000000; i++)
                _ = RhNewObject((void*)typeof(ValueTuple<double, double>).TypeHandle.Value);
            stopWatch.Stop();
            Console.WriteLine("NewDoubleTuple: " + stopWatch.Elapsed);
        }
    }
}

namespace System.Runtime
{
    public class RuntimeImportAttribute : Attribute
    {
        public string DllName { get; }
        public string EntryPoint { get; }
        public RuntimeImportAttribute(string entry) => EntryPoint = entry;
    }
}

Benchmark results (on Raspberry Pi):

Main PR

NewIntArray: 00:00:00.0255011
NewIntArray: 00:00:00.0254985
NewIntArray: 00:00:00.0199294
NewIntArray: 00:00:00.0157785
NewIntArray: 00:00:00.0158397
NewIntArray: 00:00:00.0157611
NewIntArray: 00:00:00.0158823
NewIntArray: 00:00:00.0157390
NewIntArray: 00:00:00.0157586
NewIntArray: 00:00:00.0158148
NewDoubleArray: 00:00:00.0634320
NewDoubleArray: 00:00:00.0634249
NewDoubleArray: 00:00:00.0634018
NewDoubleArray: 00:00:00.0634123
NewDoubleArray: 00:00:00.0633429
NewDoubleArray: 00:00:00.0633701
NewDoubleArray: 00:00:00.0633932
NewDoubleArray: 00:00:00.0633275
NewDoubleArray: 00:00:00.0633318
NewDoubleArray: 00:00:00.0633482
NewIntTuple: 00:00:00.0142524
NewIntTuple: 00:00:00.0142566
NewIntTuple: 00:00:00.0142554
NewIntTuple: 00:00:00.0142424
NewIntTuple: 00:00:00.0142489
NewIntTuple: 00:00:00.0142521
NewIntTuple: 00:00:00.0142627
NewIntTuple: 00:00:00.0142432
NewIntTuple: 00:00:00.0142473
NewIntTuple: 00:00:00.0142491
NewDoubleTuple: 00:00:00.0269727
NewDoubleTuple: 00:00:00.0270101
NewDoubleTuple: 00:00:00.0269876
NewDoubleTuple: 00:00:00.0269796
NewDoubleTuple: 00:00:00.0269837
NewDoubleTuple: 00:00:00.0269709
NewDoubleTuple: 00:00:00.0270441
NewDoubleTuple: 00:00:00.0269772
NewDoubleTuple: 00:00:00.0269737
NewDoubleTuple: 00:00:00.0269802

NewIntArray: 00:00:00.0266514
NewIntArray: 00:00:00.0267529
NewIntArray: 00:00:00.0264482
NewIntArray: 00:00:00.0173164
NewIntArray: 00:00:00.0164856
NewIntArray: 00:00:00.0164668
NewIntArray: 00:00:00.0165303
NewIntArray: 00:00:00.0164860
NewIntArray: 00:00:00.0164870
NewIntArray: 00:00:00.0164756
NewDoubleArray: 00:00:00.0513089
NewDoubleArray: 00:00:00.0513532
NewDoubleArray: 00:00:00.0513423
NewDoubleArray: 00:00:00.0513160
NewDoubleArray: 00:00:00.0513266
NewDoubleArray: 00:00:00.0512910
NewDoubleArray: 00:00:00.0512942
NewDoubleArray: 00:00:00.0513139
NewDoubleArray: 00:00:00.0512965
NewDoubleArray: 00:00:00.0512951
NewIntTuple: 00:00:00.0144171
NewIntTuple: 00:00:00.0144348
NewIntTuple: 00:00:00.0144299
NewIntTuple: 00:00:00.0143846
NewIntTuple: 00:00:00.0144250
NewIntTuple: 00:00:00.0143741
NewIntTuple: 00:00:00.0143745
NewIntTuple: 00:00:00.0144070
NewIntTuple: 00:00:00.0144361
NewIntTuple: 00:00:00.0144255
NewDoubleTuple: 00:00:00.0162535
NewDoubleTuple: 00:00:00.0162460
NewDoubleTuple: 00:00:00.0162538
NewDoubleTuple: 00:00:00.0162349
NewDoubleTuple: 00:00:00.0162483
NewDoubleTuple: 00:00:00.0162368
NewDoubleTuple: 00:00:00.0162206
NewDoubleTuple: 00:00:00.0162730
NewDoubleTuple: 00:00:00.0163367
NewDoubleTuple: 00:00:00.0162350

dotnet-policy-service · 2024-08-06T10:06:40Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

filipnavara · 2024-08-06T10:29:13Z

Closing for now, the benchmark is flawed.

filipnavara · 2024-08-06T11:56:59Z

Updated the OP with a fixed benchmark and more meaningful numbers. I'll will tinker a bit more with it.

filipnavara · 2024-08-06T12:52:30Z

I rewrote the RequiresAlign8 getter by exploiting a bit trick used in HasComponentSize. Now it beats the old version in the benchmarks in all cases.

Benchmark results (on Raspberry Pi):

Main PR

NewIntArray: 00:00:00.0253658
NewIntArray: 00:00:00.0255230
NewIntArray: 00:00:00.0251697
NewIntArray: 00:00:00.0221124
NewIntArray: 00:00:00.0156506
NewIntArray: 00:00:00.0157653
NewIntArray: 00:00:00.0157124
NewIntArray: 00:00:00.0157753
NewIntArray: 00:00:00.0157293
NewIntArray: 00:00:00.0156424
NewDoubleArray: 00:00:00.0631696
NewDoubleArray: 00:00:00.0631607
NewDoubleArray: 00:00:00.0631175
NewDoubleArray: 00:00:00.0631376
NewDoubleArray: 00:00:00.0632945
NewDoubleArray: 00:00:00.0631229
NewDoubleArray: 00:00:00.0631233
NewDoubleArray: 00:00:00.0630915
NewDoubleArray: 00:00:00.0631253
NewDoubleArray: 00:00:00.0631230
NewIntTuple: 00:00:00.0142051
NewIntTuple: 00:00:00.0142171
NewIntTuple: 00:00:00.0142024
NewIntTuple: 00:00:00.0141806
NewIntTuple: 00:00:00.0142125
NewIntTuple: 00:00:00.0142099
NewIntTuple: 00:00:00.0141944
NewIntTuple: 00:00:00.0141997
NewIntTuple: 00:00:00.0142020
NewIntTuple: 00:00:00.0141909
NewDoubleTuple: 00:00:00.0269583
NewDoubleTuple: 00:00:00.0269372
NewDoubleTuple: 00:00:00.0269473
NewDoubleTuple: 00:00:00.0269310
NewDoubleTuple: 00:00:00.0269361
NewDoubleTuple: 00:00:00.0269333
NewDoubleTuple: 00:00:00.0269077
NewDoubleTuple: 00:00:00.0269356
NewDoubleTuple: 00:00:00.0269354
NewDoubleTuple: 00:00:00.0269354

NewIntArray: 00:00:00.0230733
NewIntArray: 00:00:00.0237831
NewIntArray: 00:00:00.0234446
NewIntArray: 00:00:00.0232283
NewIntArray: 00:00:00.0143300
NewIntArray: 00:00:00.0143772
NewIntArray: 00:00:00.0143603
NewIntArray: 00:00:00.0143911
NewIntArray: 00:00:00.0143334
NewIntArray: 00:00:00.0144492
NewDoubleArray: 00:00:00.0508229
NewDoubleArray: 00:00:00.0509037
NewDoubleArray: 00:00:00.0510827
NewDoubleArray: 00:00:00.0505703
NewDoubleArray: 00:00:00.0506560
NewDoubleArray: 00:00:00.0505697
NewDoubleArray: 00:00:00.0507030
NewDoubleArray: 00:00:00.0506166
NewDoubleArray: 00:00:00.0505482
NewDoubleArray: 00:00:00.0505669
NewIntTuple: 00:00:00.0138182
NewIntTuple: 00:00:00.0138272
NewIntTuple: 00:00:00.0137903
NewIntTuple: 00:00:00.0138285
NewIntTuple: 00:00:00.0138366
NewIntTuple: 00:00:00.0138224
NewIntTuple: 00:00:00.0137791
NewIntTuple: 00:00:00.0138458
NewIntTuple: 00:00:00.0138181
NewIntTuple: 00:00:00.0138305
NewDoubleTuple: 00:00:00.0154690
NewDoubleTuple: 00:00:00.0155391
NewDoubleTuple: 00:00:00.0154707
NewDoubleTuple: 00:00:00.0155383
NewDoubleTuple: 00:00:00.0154718
NewDoubleTuple: 00:00:00.0155317
NewDoubleTuple: 00:00:00.0154563
NewDoubleTuple: 00:00:00.0155298
NewDoubleTuple: 00:00:00.0154692
NewDoubleTuple: 00:00:00.0155386

jkotas · 2024-08-06T15:56:48Z

How do we store RequiresAlign8 for arrays of value types with this change? If I am reading the code correctly, RequiresAlign8 is always going to be false for arrays with this change.

filipnavara · 2024-08-06T16:04:03Z

How do we store RequiresAlign8 for arrays of value types with this change?

We don’t. We just check the element type of the array for IsValueType && RequiresAlign8. The array allocation path is separate so the cost of the check is moved there. I meant to add Debug.Assert(!IsArray) into the getter but forgot about it.

jkotas · 2024-08-06T16:09:44Z

If the getter is not supposed to be called on arrays, we can omit the HasComponentSizeFlag tricks from it and just test the single bit.

filipnavara · 2024-08-06T16:11:57Z

The HasComponentSizeFlag check is still needed for string.

jkotas · 2024-08-06T16:15:09Z

I would expect that string allocations to be always special cased. Where is the RequiresAlign8 checked for string allocations?

filipnavara · 2024-08-06T17:50:42Z

I would expect that string allocations to be always special cased. Where is the RequiresAlign8 checked for string allocations?

Presumably you are right. I checked all the code paths and couldn’t find one that would be hit with string. Moreover, there’s already an assert in RhNewObject to disallow strings. I’ll simplify the check.

filipnavara · 2024-08-06T19:43:10Z

This should definitely run through the NativeAOT outerloop tests if the PR looks reasonable in other aspects.

jkotas · 2024-08-06T19:50:41Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-08-06T19:50:57Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas

LGTM!

jkotas · 2024-08-07T00:59:33Z

A lot of tests on linux-arm crashing with stackoverflow:

ld_linux_armhf_so!_tls_get_addr+0x5
baseservices!S_P_CoreLib_System_Threading_Lock::EnterAndGetCurrentThreadId+0x16 [/_/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs @ 83] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner_Cctor::GetCctor+0x2e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 279] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::EnsureClassConstructorRun+0x2a [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 66] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::CheckStaticClassConstructionReturnGCStaticBase+0x10 [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 36] 
baseservices!S_P_CoreLib_Internal_Runtime_ThreadStatics::AllocateThreadStaticStorageForType+0x7e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @ 140] 
baseservices!S_P_CoreLib_Internal_Runtime_ThreadStatics::GetUninlinedThreadStaticBaseForTypeSlow+0x146 [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @ 110] 
baseservices!S_P_CoreLib_System_Threading_Lock::EnterAndGetCurrentThreadId+0x16 [/_/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs @ 83] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner_Cctor::GetCctor+0x2e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 279] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::EnsureClassConstructorRun+0x2a [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 66] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::CheckStaticClassConstructionReturnGCStaticBase+0x10 [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 36] 
baseservices!S_P_CoreLib_Internal_Runtime_ThreadStatics::AllocateThreadStaticStorageForType+0x7e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @ 140] 
baseservices!S_P_CoreLib_Internal_Runtime_ThreadStatics::GetUninlinedThreadStaticBaseForTypeSlow+0x146 [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @ 110] 
baseservices!S_P_CoreLib_System_Threading_Lock::EnterAndGetCurrentThreadId+0x16 [/_/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs @ 83] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner_Cctor::GetCctor+0x2e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 279] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::EnsureClassConstructorRun+0x2a [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 66] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::CheckStaticClassConstructionReturnGCStaticBase+0x10 [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 36] 
baseservices!S_P_CoreLib_Internal_Runtime_ThreadStatics::AllocateThreadStaticStorageForType+0x7e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @ 140] 
baseservices!S_P_CoreLib_Internal_Runtime_ThreadStatics::GetUninlinedThreadStaticBaseForTypeSlow+0x146 [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @ 110] 
baseservices!S_P_CoreLib_System_Threading_Lock::EnterAndGetCurrentThreadId+0x16 [/_/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs @ 83] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner_Cctor::GetCctor+0x2e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 279] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::EnsureClassConstructorRun+0x2a [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 66] 
baseservices!S_P_CoreLib_System_Runtime_CompilerServices_ClassConstructorRunner::CheckStaticClassConstructionReturnGCStaticBase+0x10 [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs @ 36] 
baseservices!S_P_CoreLib_Internal_Runtime_ThreadStatics::AllocateThreadStaticStorageForType+0x7e [/_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @ 140] 
...

filipnavara · 2024-08-07T02:28:04Z

A lot of tests on linux-arm crashing with stackoverflow:

It was a very stupid mistake in the last change.

MichalStrehovsky · 2024-08-07T02:35:42Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-08-07T02:35:51Z

Azure Pipelines successfully started running 1 pipeline(s).

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Aug 6, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 6, 2024

Move RequiresAlign8 flag from RareFlags into ExtendedFlags

3b37862

filipnavara force-pushed the align8flag branch from 9c54401 to 3b37862 Compare August 6, 2024 10:08

Fix cut & paste error when cleaning up the comments

1c65433

filipnavara closed this Aug 6, 2024

build-analysis bot mentioned this pull request Aug 6, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

Optimize the RequiresAlign8 check

3cc9ece

filipnavara reopened this Aug 6, 2024

This was referenced Aug 6, 2024

TimeProviderTests.TestProviderTimer failed in CI #103459

Closed

NuGet restore failing with NullReferenceException #103823

Closed

filipnavara marked this pull request as ready for review August 6, 2024 13:37

filipnavara requested a review from MichalStrehovsky as a code owner August 6, 2024 13:37

build-analysis bot mentioned this pull request Aug 6, 2024

Roslyn analyzer throws error AD0001 NullReferenceException dotnet/dnceng#3305

Open

3 tasks

github-actions bot mentioned this pull request Aug 6, 2024

106010 MichalStrehovsky/rt-sz#56

Closed

build-analysis bot mentioned this pull request Aug 6, 2024

[NativeAOT] Object synchronization method was called from an unsynchronized block of code. #104340

Closed

Simplify RequiresAlign8 even further, add debug assert

0cf9dd3

jkotas approved these changes Aug 6, 2024

View reviewed changes

Fix flipped condition in assertion

57ad074

MichalStrehovsky merged commit 4153092 into dotnet:main Aug 7, 2024
104 of 109 checks passed

filipnavara deleted the align8flag branch August 7, 2024 07:31

github-actions bot locked and limited conversation to collaborators Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NativeAOT] Move RequiresAlign8 flag from RareFlags into ExtendedFlags #106010

[NativeAOT] Move RequiresAlign8 flag from RareFlags into ExtendedFlags #106010

filipnavara commented Aug 6, 2024 •

edited

Loading

dotnet-policy-service bot commented Aug 6, 2024

filipnavara commented Aug 6, 2024

filipnavara commented Aug 6, 2024 •

edited

Loading

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

filipnavara commented Aug 6, 2024 •

edited

Loading

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

azure-pipelines bot commented Aug 6, 2024

jkotas left a comment

jkotas commented Aug 7, 2024 •

edited

Loading

filipnavara commented Aug 7, 2024

MichalStrehovsky commented Aug 7, 2024

azure-pipelines bot commented Aug 7, 2024

[NativeAOT] Move RequiresAlign8 flag from RareFlags into ExtendedFlags #106010

[NativeAOT] Move RequiresAlign8 flag from RareFlags into ExtendedFlags #106010

Conversation

filipnavara commented Aug 6, 2024 • edited Loading

dotnet-policy-service bot commented Aug 6, 2024

filipnavara commented Aug 6, 2024

filipnavara commented Aug 6, 2024 • edited Loading

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

filipnavara commented Aug 6, 2024 • edited Loading

filipnavara commented Aug 6, 2024

jkotas commented Aug 6, 2024

azure-pipelines bot commented Aug 6, 2024

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Aug 7, 2024 • edited Loading

filipnavara commented Aug 7, 2024

MichalStrehovsky commented Aug 7, 2024

azure-pipelines bot commented Aug 7, 2024

filipnavara commented Aug 6, 2024 •

edited

Loading

filipnavara commented Aug 6, 2024 •

edited

Loading

filipnavara commented Aug 6, 2024 •

edited

Loading

jkotas commented Aug 7, 2024 •

edited

Loading